feat(auth): Add /userinfo endpoint for OIDC #52493
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #52493 +/- ##
==========================================
- Coverage 79.35% 77.74% -1.61%
==========================================
Files 4914 4893 -21
Lines 206216 206110 -106
Branches 35250 35247 -3
==========================================
- Hits 163634 160249 -3385
- Misses 37569 40708 +3139
- Partials 5013 5153 +140
|
EricHasegawa
force-pushed
the
EricHasegawa/add-oidc-to-token-endpoint
branch
from
dc5ceec to
1775f42
Compare
EricHasegawa
force-pushed
the
EricHasegawa/add-oidc-to-token-endpoint
branch
from
a2bcaf9 to
932084f
Compare
EricHasegawa
force-pushed
the
EricHasegawa/create-userinfo
branch
from
8cb951d to
da08a3c
Compare
| "name": user.name, | ||
| "avatar_type": user.avatar_type, | ||
| "avatar_url": user.avatar_url, | ||
| "date_joined": user.date_joined, |
There was a problem hiding this comment.
Generally our API outputs properties with camelBacked identifiers. Is there a reason to diverge from that convention here?
There was a problem hiding this comment.
yeah - the OAuth/OIDC requires snake case in order for this to be considered a valid implementation - here's a thread discussing this exact thing.
However - this is only for the required OAuth stuff like client_id, access_token etc. So technically we could return those in snake case, and return the custom userinfo stuff in camelCase, but, I figured that would be even more confusing. LMK if you prefer it that way though and I can change it over!
There was a problem hiding this comment.
Fair point about the OIDC spec having standards for response keys. 🤔 Having two conventions in the same response is clunky. Lets go forward with what you have. Having consistency within the response which needs to follow the standard for some properties seems more important than our local API response key conventions.
| user_output.update(profile_details) | ||
| if "email" in scopes: | ||
| email = UserEmail.objects.get(user=user) | ||
| email_details = {"email": email.email, "email_verified": email.is_verified} |
There was a problem hiding this comment.
The email_verified attribute is generally serialized as is_verified. While It is unfortunately a snake cased identifier we should try to use consistent names across various endpoints.
There was a problem hiding this comment.
I named this one email_verified since it's consistent with standard OIDC language (example from Auth0).
Also - this response contains more details on the user than just email stuff, if we just have is_verified do you think clients might be unsure as to what exactly is verified?
On the other hand, maybe is_verified, by extension, implies that the user is verified, and not just the email, plus we would be consistent with our codebase, rather than the OAuth standards, so there's a trade-off here.
I'd leave it this way but I'm good either way - lmk if you want me to change it over still and I will!
There was a problem hiding this comment.
Good points. Combined with the OIDC conventions discussed above, let's leave this as you have it.
| "name": user.name, | ||
| "avatar_type": user.avatar_type, | ||
| "avatar_url": user.avatar_url, | ||
| "date_joined": user.date_joined, |
There was a problem hiding this comment.
Fair point about the OIDC spec having standards for response keys. 🤔 Having two conventions in the same response is clunky. Lets go forward with what you have. Having consistency within the response which needs to follow the standard for some properties seems more important than our local API response key conventions.
| user_output.update(profile_details) | ||
| if "email" in scopes: | ||
| email = UserEmail.objects.get(user=user) | ||
| email_details = {"email": email.email, "email_verified": email.is_verified} |
There was a problem hiding this comment.
Good points. Combined with the OIDC conventions discussed above, let's leave this as you have it.
Implements the
/userinfoendpoint for OIDC compliance (initial spec is here).Note that anyone can call this endpoint - the authentication/authorization comes from the bearer token they pass in.